Use subtests where appropriate everywhere - #2391

Hi @mdboom - I asked Codex to turn all of the findings from its review of #2391 into concrete fixes in the experimental draft PR #2479. My plan was to review the changes myself first, then offer the individual commits as possible cherry-picks rather than add a long list of abstract suggestions here. Please feel free to cherry-pick, adapt, or discard each item independently.

The complete set passed this CI run. That run was before I reverted 1344064. I pushed the revert afterward only to make it clear that I do not want ci/ to depend on code in toolshed/; it was not a response to a CI failure.

The three changes I looked at more closely

1. Recognize native pytest subtest outcomes in the skipped-test report

Commit e7f8017 teaches toolshed/find_skipped_tests.py about SUBPASSED, SUBFAILED, SUBSKIPPED, and the other native subtest outcomes.

The important case is an all-skipped subtest: pytest emits one or more SUBSKIPPED lines and then reports the parent test as PASSED. Treating that final parent pass as evidence that the test executed would incorrectly remove it from the "always skipped" report. The change tracks whether subtest outcomes were seen, ignores the synthetic parent pass in that case, and still treats a mixed skipped/passed set as having executed.

There is one test-ownership question here. The parser belongs to toolshed/, and I think ci/ should remain independent of it. Commit 1344064 moved the small regression test into ci/tools/tests so normal CI would collect it; I reverted that move in d6376d5, leaving the test back under toolshed/ and outside normal CI collection.

My inclination is to delete this small regression test. If you think it is worth retaining, I think it should live under toolshed/tests/; we could then decide whether ci-nightly.yml should explicitly run:

pytest -v --noconftest toolshed/tests

I would be interested in which of those two directions you prefer. The parser fix itself is independent of that choice.

2. Use the documented Fermi boundary for the power-limit getter

Commit 57e1a4c changes only the getter test from Kepler to Fermi.

The native NVML documentation for nvmlDeviceGetPowerManagementLimit says the getter is supported on Fermi or newer fully supported devices. The nearby default-limit and constraints queries, and the setters, are documented as Kepler or newer. Using the setter's Kepler boundary for the getter would therefore skip a valid Fermi case.

3. Use readable device labels in subtest reports

Commit 978d282 changes report metadata such as:

SUBSKIPPED(device=<cuda.core.system._device.Device object at 0x...>)
SUBSKIPPED(device=125060957155008)

to:

SUBSKIPPED(device_index=0)

subtests.test() uses its keyword arguments as report descriptions, not as parametrization. Python still evaluates device.index before entering the context, just as it would any argument; after that lookup, the value only controls the description. It does not create a different pytest node ID or replace the outer device that the test body uses. I think the index is better metadata because object addresses and raw NVML handles are opaque and process-specific, while device_index=0 is concise and directly comparable across jobs. The index is not intended as a persistent hardware identity across reboots; the improvement is clearer, repeatable CI output.

The remaining findings

4. Give every fan its own subtest

Commits 0d565c1 and 3e8d151 are best considered together.

There is a concrete coverage bug in the original fan loop: it obtains each fan in turn, but the assertions and setter/getter checks are outside the loop, so they run only once against the final fan. The first commit moves the complete check inside the loop and gives each (device, fan_index) pair its own subtest, so every fan is actually exercised and one fan's result does not suppress the others. The follow-up gives the num_fans query its own per-device subtest, so an unsupported or zero-fan device is reported for that device without preventing later devices from being considered.

5. Isolate independent inner loop cases

Commit 69009b0 applies the same principle more broadly: a broad per-device subtest does not provide completeness when several independent, potentially skipping cases are nested inside it.

It splits or flattens the relevant boundaries for affinity scopes/APIs, CPU-affinity results, compute modes, clock types, temperature thresholds and sensors, and NVLink links/APIs. A skip or failure in one independent item is then reported for that item while the remaining items can still run. This seems like a useful middle ground: keep the loops simple, but put the subtest boundary around the unit of coverage we actually want to preserve.

6. Narrow the cooler unsupported guard

Commit 07e493b limits unsupported_before(...) to the device.cooler lookup it is intended to guard.

Keeping the subsequent type checks and signal_type/target property validation outside that guard prevents an unexpected NotSupported from those later operations from being converted into a skip. In other words, known architectural absence remains a skip, while a regression in the returned object remains visible as a failure.

Cherry-pick notes

• 0d565c1 and 3e8d151 belong together.
• 2adcc74 is only the Ruff SIM117 cleanup needed after the related context-manager changes; it is not a separate behavioral proposal.
• 978d282 touches many of the same subtest lines as the other changes, so it will likely apply most cleanly after whichever behavioral commits you choose.
• 1344064 and d6376d5 record the test-location experiment and its revert; they cancel each other and neither is intended for cherry-picking.
• The merge commits in Exercise suggested subtest changes for #2391 #2479 are branch maintenance and are not intended for cherry-picking.

If you want the complete set, the original order of the proposed commits is:

0d565c1a94 e7f8017349 69009b0410 57e1a4c42d 07e493b689 3e8d151f6a 978d282d76 2adcc74eb5

1. Recognize native pytest subtest outcomes in the skipped-test report

I think updating the skipped test finder makes sense. I think adding testing to toolshed utilities is out-of-scope here, though. If we want to do that, let's do it as follow-on work.

2. Use the documented Fermi boundary for the power-limit getter

I don't think we should change any of the limits from the existing code on main. The documentation about when features are available is largely wrong, and most of these limits were discovered through trial-and-error by submitting to QA. I worry that messing them will cause unnecessary cycles with QA again.

3, 4, 5, and 6 seem fine.

Why not just push the changes to this PR and I will give it a final review?

Hi @mdboom — thanks for the detailed reply on the suggestions from #2479.

I have now applied the agreed work directly to this branch. I used #2479 as a scratch area, reviewed each proposed change, and then cherry-picked or adapted the useful parts here after merging the current main. I kept the logical changes separate so they are easy to inspect, adjust, or drop independently.

Merge from main

• b9ecde4 merges the current main into this branch before applying the follow-up changes. The conflict resolutions keep this PR's pytest 9 minimum, main's new cuda-python-test-helpers dependency and platform helpers, main's newer system-device tests, and main's removal of the obsolete NVLINK_MAX_LINKS test.

Follow-up commits

• 910462a gives every physical fan its own subtest, so a failure for one fan does not prevent the remaining fans from being exercised.

• 9dd3c70 teaches toolshed/find_skipped_tests.py about pytest's SUB* outcomes. In particular, a parent reported as PASSED no longer hides the fact that all of its subtests skipped. Following your preference, the final branch does not add a toolshed test under ci/ (or elsewhere).

• b7f469c separates independent inner cases — including compute modes, affinity APIs/scopes, clocks, temperature queries, and NVLink queries — so one skip or failure does not truncate unrelated coverage.

• 35ca799 narrows the cooler unsupported_before scope to the API call itself, leaving the result validation outside that context.

• 7ccd34e queries and validates the fan count in its own subtest. A failure there remains local to that device instead of aborting the rest of the test.

• 0f67e6b replaces raw device objects/handles in subtest metadata with scalar device indices. A raw object or handle representation can include a process-specific address; device_index=0 is concise, searchable, and much easier to compare across jobs.

• a014155 combines adjacent context managers when they intentionally cover the same region.

• 2adfe8e preserves the support behavior already present on main: the power-limit getter keeps unsupported_before(device, None), while the setter remains guarded as Kepler-or-newer.

• e087038 adds an explicit Kepler guard to the newly independent memory-affinity subtest. Before the split it implicitly stopped when the preceding CPU-affinity call skipped; after the split it needs its own guard.

• a3dc33d validates temperature, thermal-settings, and NVLink-count results before assigning them to values consumed outside their subtests. This prevents a failed type assertion from leaking an invalid value into later cases.

Validation

• A local clean CUDA 13.3 build completed successfully.
• The full test run passed for pathfinder, both bindings configurations, bindings Cython tests, and core Cython tests.
• The core Python run had one expected failure (due to my local driver), everything else passed.
• After the last two audit fixes, the focused affinity/temperature/NVLink selection passed: 8 tests passed, 8 architecture-dependent subtests skipped, and 204 subtests passed.
• The parser smoke checks and all non-network pre-commit hooks pass.

Please feel free to adapt or discard any individual commit if you prefer a different shape.